local echo (4.5/n): Prep for creating outbox messages on send #1509
Conversation
PIG208
added
the
integration review
There was a problem hiding this comment.
Thanks, and thanks @chrisbobbe for the previous reviews! Generally this looks good; a few comments.
lib/api/model/model.dart
Outdated
| return this; | ||
| } | ||
|
|
||
| if (_value == kNoTopicTopic || _value.isEmpty) { |
There was a problem hiding this comment.
The "(no topic)" case here looks like it started more recently than the other, at FL 370:
https://zulip.com/api/changelog
lib/api/model/model.dart
Outdated
| /// This assumes that the topic is originally for a send-message | ||
| /// request — such a topic must be constructed from a string without | ||
| /// leading/trailing whitespace. |
There was a problem hiding this comment.
Hmm, is that a requirement of the server endpoint? I don't see it here:
https://zulip.com/api/send-message#parameter-topic
Fine for this method to have this requirement; just shouldn't attribute it to the server.
test/api/model/model_test.dart
Outdated
| check(() => doCheck(eg.t(''), eg.t(''), 333)) | ||
| .throws<void>(); |
There was a problem hiding this comment.
The throws here would end up catching an error from the check itself failing. That doesn't seem intended — I gather what you want is to check that interpretAsServer itself throws.
So for example this test as written still passes if you change this 333 to 334, at which point interpretAsServer doesn't throw but just returns an answer different from eg.t('').
lib/api/model/model.dart
Outdated
| /// `store.realmEmptyTopicDisplayName`. | ||
| /// | ||
| /// See also: https://zulip.com/api/send-message#parameter-topic | ||
| TopicName interpretAsServer({ |
There was a problem hiding this comment.
This name doesn't feel right to me — "interpret as server" sounds like it's trying to interpret the topic name as being a server (whatever that would mean).
How about processLikeServer?
test/example_data.dart
Outdated
| editTimestamp: editTimestamp ?? 1234567890, // TODO generate timestamp | ||
| editTimestamp: editTimestamp ?? utcTimestamp(), |
There was a problem hiding this comment.
I guess I commented on this change earlier today over on #1498 where Chris had cherry-picked it:
#1498 (comment)
PIG208
force-pushed
the
pr-echo-4.5
branch
2 times, most recently
from
5296d17 to
815fff7
Compare
|
Thanks! Updated the PR. |
lib/api/model/model.dart
Outdated
| /// This assumes that the topic is constructed from a string without | ||
| /// leading/trailing whitespace. |
There was a problem hiding this comment.
| /// This assumes that the topic is constructed from a string without | |
| /// leading/trailing whitespace. | |
| /// This returns the [TopicName] the server would be predicted to include | |
| /// in a message object resulting from sending to this [TopicName] | |
| /// in a [sendMessage] request. | |
| /// | |
| /// This [TopicName] is required to have no leading or trailing whitespace. |
The other part is needed too (I think it came from a comment @chrisbobbe made on the original PR): it says how this topic relates to the hypothetical scenario where the returned topic appears in a message object from the server.
lib/api/model/model.dart
Outdated
| return TopicName(kNoTopicTopic); | ||
| } | ||
|
|
||
| // TODO(#1250): This assumes that the 'empty_topic_name' client capability |
There was a problem hiding this comment.
to update now that #1365 is merged; the doc above, too
The point of this helper is to replicate what a topic sent from the client will become, after being processed by the server. This important when trying to create a local copy of a stream message, whose topic can get translated when it's delivered by the server.
This will be the same as `DateTime.timestamp()` in live code (therefore the NFC). For testing, utcNow uses a clock instance that can be controlled by FakeAsync. We could have made call sites of `DateTime.now()` use it too, but those for now don't need it for testing.
|
Updated this. Thanks! |
|
Thanks! Looks good; merging. |
This prepares for #1472, toward #1441.
Requested by @chrisbobbe here.